Fix MIDI pickup crossing detection never firing - #3835
Conversation
Pickup mode has a second acceptance path, next to the tolerance test, for the case where a fast fader move makes consecutive MIDI values skip over the software value. Two separate defects meant it never ran. While waiting for pickup, midiPickupTryApply returned early without recording the value, so the history deque stayed empty and the size() >= 2 guard in midiPickupShouldApply was never satisfied. And prevMidi read back(), which is the value just appended, so the bracket test reduced to midiValue == currentValue, already covered by the tolerance test above it. Either defect alone disables the path; both are fixed here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| if ( !midiPickupShouldApply ( midiValue, currentValue, tolerance, tempPickup ) ) | ||
| { | ||
| // keep this value: the next message needs it to detect a crossing | ||
| pickupBuffer = tempPickup; |
There was a problem hiding this comment.
Where's pickupBuffer defined and used? (Maybe hint that in the comment - but if it's obvious, this comment is fine.)
There was a problem hiding this comment.
@mcfnord please have a look at the PRs you/your agent looked at and comment...
There was a problem hiding this comment.
🤖 AI: pickupBuffer is a parameter name, not an object: midiPickupTryApply takes the caller's deque by reference, and midiPickupInactivityCheck takes the same deque to clear it. The objects behind it are the two per-channel deques in MidiPickupState: SetFaderLevel passes recentFader, SetPanValue passes recentPan. The name is unchanged from 97799184; this PR only adds the write-back under the comment.
Naming the referent:
// still waiting: keep this value in the channel's recentFader/recentPan
// history so the next message can detect a crossingNot pushed — the branch still carries the one-line form.
Note
📡 STAND BY FOR AN LLM-AUTHORED MESSAGE.
MIDI pickup mode has a second acceptance path beside the tolerance test, for when a fast fader move makes consecutive MIDI values skip straight over the software value:
It has never run. There are two independent defects, and either one alone is enough to disable it.
1. The history is never recorded while waiting.
midiPickupTryApplybuilds a temporary copy, tests it, and on failure returns early — jumping over the// Update the pickup bufferblock at the end of the function. So whilewaitingForPickupis true, which is exactly the state the history exists to serve,pickupBufferstays empty. The largest deque ever passed tomidiPickupShouldApplyis 1, sosize() >= 2is never satisfied.2.
prevMidiis not the previous value.recentMidiValues.back()is the elementmidiValuewas justpush_back'd as, soprevMidi == midiValueand the bracket test reduces tomidiValue == currentValue— already covered by the tolerance test three lines above. Sweeping(prev, current, new)over 0..100 outside tolerance, the current code detects 0 of 323400 genuine crossings.The
size() >= 2guard is itself the evidence of intent: you only need two elements if you mean to look past the last one.What it costs a user
CSoundBase::ParseMIDIMessagemaps CC 0..127 onto fader 0..AUD_MIX_FADER_MAX(100), so one CC step is 0.79 fader units against aMIDI_PICKUP_TOLERANCEof 2. Slow moves are picked up by the tolerance test and the bug stays invisible. Fast moves are where it shows: sweeping a hardware fader past a software fader sitting at 50, the fader stays dead at several speeds until the user slows down and lands within ±2 units.The two middle columns are the reason this PR changes two things rather than one: fixing either defect on its own changes nothing measurable.
Checks
Harness compiles the two patched functions verbatim out of
audiomixerboard.cpprather than a re-typed copy, so what is measured is what is shipped.clang-format14.0.6 reports no change; compiles clean under-Wall -Wextra.What I did not do
I have not run a patched Jamulus with a physical MIDI controller — the behaviour above is from the extracted functions, not from hardware. If someone with a controller can confirm the fast-sweep case, that would close the loop.
One related thing I noticed and deliberately left alone:
g_midiPickupWaitingForPickupis a single flag per channel shared by the fader and the pan, while the history deques and timestamps beside it inMidiPickupStateare correctly per-control (recentFader/recentPan,lastMidiTimeFader/lastMidiTimePan). That means picking up the pan also clears the fader's waiting state, and either control's inactivity timeout re-arms both. It needs two flags rather than one, which is a different change from this one, so I have not folded it in. Happy to open it separately if you would like.CHANGELOG: Bugfix: MIDI pickup mode now correctly detects a fader crossing the software value during fast moves.